-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(windows-installer): add client telemetry opt out option #210
Conversation
4ce26fc
to
b757b26
Compare
Signed-off-by: Jericho Tolentino <68654047+jericht@users.noreply.github.com>
b757b26
to
5f28806
Compare
deadline_client_config_path = deadline.client.config.config_file.CONFIG_FILE_PATH | ||
if not deadline_client_config_path.startswith("~"): | ||
raise InstallerFailedException( | ||
f"Cannot opt out of telemtry: Expected Deadline client config file path to start with a tilde (~), but got: {deadline_client_config_path}\n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: telemtry
→ telemetry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a spelling mistake and we need to add the else
case.
if telemetry_opt_out: | ||
logging.info("Opting out of client telemetry") | ||
update_deadline_client_config( | ||
user=user_name, | ||
settings={"telemetry.opt_out": "true"}, | ||
) | ||
logging.info("Opted out of client telemetry") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we need an else
which should set telemetry.opt_out
to `false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense, but this is not the same behavior as the Linux installer: https://github.com/casillas2/deadline-cloud-worker-agent/blob/58ce63d150d4df8229a7f418e8731a9f11411f23/src/deadline_worker_agent/installer/install.sh#L446-L450
Should I update the Linux installer to do the same? (If the user does not pass any --telemetry-opt-out
, then we silently opt them in?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose another alternative is that we:
- add another argument
--telemetry-opt-in
which is mutually-exclusive with--telemetry-opt-out
- when the installer detects a pre-existing config file, we'd not modify anything unless one of the arguments are passed
This makes re-installation behavior explicit and allows keeping the prior telemetry setting.
Maybe we should follow the Linux behavior for now and move this out-of-scope for the current PR.
What was the problem/requirement? (What/Why)
The Windows installer is missing the option to opt out of client telemetry. The Linux installer has this option, so we need to add it to the Windows one as well.
What was the solution? (How)
Add option to opt out of client telemetry
--telemetry-opt-out
. When this option is provided, the installer will use the Deadline Client's config API to update thetelemetry.opt_out
setting totrue
.What is the impact of this change?
Users can opt-out of client telemetry through the Windows installer
How was this change tested?
Installer Output
Note: The
icacls
output between the opting out of client telemetry is output bydeadline-cloud
and will be removed in a future release ofdeadline-cloud
.Deadline Client config file
Was this change documented?
No
Is this a breaking change?
No